Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve TagEncoding::Niche docs, sanity check, and UB checks #133681

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 30, 2024

Turns out the niche_variants range can actually contain the untagged_variant. We should report this as UB in Miri, so this PR implements that.

Also rename partially_check_layout to layout_sanity_check for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix #126267.

@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2024

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

The transmute logic seems to be calling tag_for_variant on uninhabited variants. Those variants do not have a tag, so... it is not clear to me what the logic should be in that case. Cc @jswrenn , I hope what I did in the last commit makes sense.

@rust-log-analyzer

This comment has been minimized.

tests/ui/transmutability/uninhabited.rs Outdated Show resolved Hide resolved
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with or without the nit 🙂

compiler/rustc_abi/src/lib.rs Show resolved Hide resolved
@wesleywiser
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 3, 2024

📌 Commit 611a991 has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 3, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 3, 2024
improve TagEncoding::Niche docs, sanity check, and UB checks

Turns out the `niche_variants` range can actually contain the `untagged_variant`. We should report this as UB in Miri, so this PR implements that.

Also rename `partially_check_layout` to `layout_sanity_check` for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix rust-lang#126267.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#132937 (a release operation synchronizes with an acquire operation)
 - rust-lang#133681 (improve TagEncoding::Niche docs, sanity check, and UB checks)
 - rust-lang#133726 (Add `core::arch::breakpoint` and test)
 - rust-lang#133768 (Remove `generic_associated_types_extended` feature gate)
 - rust-lang#133811 ([AIX] change AIX default codemodel=large)
 - rust-lang#133812 (Update wasm-component-ld to 0.5.11)
 - rust-lang#133813 (compiletest: explain that UI tests are expected not to compile by default)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6e87eb5 into rust-lang:master Dec 4, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 4, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 4, 2024
Rollup merge of rust-lang#133681 - RalfJung:niches, r=wesleywiser

improve TagEncoding::Niche docs, sanity check, and UB checks

Turns out the `niche_variants` range can actually contain the `untagged_variant`. We should report this as UB in Miri, so this PR implements that.

Also rename `partially_check_layout` to `layout_sanity_check` for better consistency with how similar functions are called in other parts of the compiler.

Turns out my adjustments to the transmutation logic also fix rust-lang#126267.
@RalfJung RalfJung deleted the niches branch December 4, 2024 06:32
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 18, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 19, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 19, 2024
Rollup merge of rust-lang#133702 - RalfJung:single-variant, r=oli-obk

Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Dec 20, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Dec 23, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
bjorn3 pushed a commit to rust-lang/rustc_codegen_cranelift that referenced this pull request Dec 28, 2024
Variants::Single: do not use invalid VariantIdx for uninhabited enums

~~Stacked on top of rust-lang/rust#133681, only the last commit is new.~~

Currently, `Variants::Single` for an empty enum contains a `VariantIdx` of 0; looking that up in the enum variant list will ICE. That's quite confusing. So let's fix that by adding a new `Variants::Empty` case for types that have 0 variants.

try-job: i686-msvc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: overflow computing relative variant idx in rustc_const_eval/src/interpret/discriminant.rs
6 participants